-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[macOS] Forward mouseDown/Up to view controller #40241
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This works around an AppKit bug in which mouseDown/mouseUp events are not correctly forwarded up the responder chain for views nested inside an NSPopover if (and only if) the macOS "Reduce Transparency" accessibility setting is enabled in the System Settings. When the above conditions are satisfied, the nested NSView receives the mouseDown:/mouseUp: call but if it delegates to the default implementation (implemented in NSResponder) mouseDown/mouseUp calls are triggered on containing views (in our case FlutterViewWrapper) but not triggered on the view controller and other responders in the responder chain until we an _NSPopoverWindow class is hit. A minimal AppKit-only (non-Flutter) repro shows this behaviour repros with even a minimal NSViewController implementation and an unmodified NSView. See: https://github.com/cbracken/PopoverRepro A radar has been filed with Apple and a copy posted to OpenRadar. See: http://www.openradar.me/FB12050037 In order to work around this bug, we override mouseDown/mouseUp in the topmost containing view of FlutterView (in our case, FlutterViewWrapper) to have the behaviour documented as the default behaviour in NSResponder's mouseDown/mouseUp documentation. In otherwords, to simply forward the call to self.nextResponder. See: https://developer.apple.com/documentation/appkit/nsresponder/1524634-mousedown Because replicating the exact configuration of a FlutterView contained in an NSPopover and System Settings that have been modified to enable the "Reduce Transparency" setting is difficult and likely error-prone in infra, we instead simulate the bug by testing that even if NSResponder's mouseDown/mouseUp method are swizzled to no-op, these calls are correctly forwarded to the next responder in the chain. If, in the future Apple does fix this issue, this workaround can be removed once Flutter's minimum supported macOS SDK is at least the version that contains the fix. Issue: flutter/flutter#115015
stuartmorgan
approved these changes
Mar 11, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Mar 11, 2023
auto-submit bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Mar 11, 2023
…gine#40241) (#122472) Roll Flutter Engine from c7894a662ae1 to 04e8d5431203 (1 revision)
sourcegraph-bot
pushed a commit
to sgtest/megarepo
that referenced
this pull request
Mar 11, 2023
…lutter/engine#40241) (#122472) Commit: 5887b430c815a7802935511120f23db7c3aa3986
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 12, 2023
jonahwilliams
added a commit
to jonahwilliams/engine
that referenced
this pull request
Mar 13, 2023
commit db8dfbe Author: jonahwilliams <jonahwilliams@google.com> Date: Mon Mar 13 10:44:36 2023 -0700 update malioc diff commit 0877a53 Merge: 04e8d80 7c5a9d5 Author: jonahwilliams <jonahwilliams@google.com> Date: Mon Mar 13 10:40:07 2023 -0700 Merge branch 'master' of github.com:flutter/engine into uv_computation commit 7c5a9d5 Author: Jackson Gardner <jacksongardner@google.com> Date: Mon Mar 13 09:54:22 2023 -0700 Use plain Uint32List objects with the fragmenter APIs. (flutter#40239) Use plain Uint32List objects with the fragmenter APIs. commit 04e8d80 Author: jonahwilliams <jonahwilliams@google.com> Date: Mon Mar 13 09:50:23 2023 -0700 fix double divide and add test commit 9b42cbc Author: jonahwilliams <jonahwilliams@google.com> Date: Mon Mar 13 09:32:48 2023 -0700 [impeller] implement GetPositionUVBuffer commit 3d545ad Author: Dan Field <dnfield@google.com> Date: Mon Mar 13 09:22:28 2023 -0700 [Impeller][Compute] Fix visual issues with heart (flutter#40240) commit 650c6e3 Author: Zachary Anderson <zanderso@users.noreply.github.com> Date: Mon Mar 13 08:45:38 2023 -0700 Revert "[Impeller] More sundry fixes to the Vulkan backend. (flutter#40244)" (flutter#40247) Revert "[Impeller] More sundry fixes to the Vulkan backend." commit 3ac895e Author: Jonah Williams <jonahwilliams@google.com> Date: Mon Mar 13 08:42:13 2023 -0700 [Impeller] support for foreground shaders on text (flutter#40193) [Impeller] support for foreground shaders on text commit bb1ca8f Author: Lasse R.H. Nielsen <lrn@google.com> Date: Mon Mar 13 13:43:04 2023 +0100 Change `extends Iterator` to using `implements` (flutter#40175) The Dart 3.0 libraries will mark Iterator with the interface class modifier, which prevents extends. It will do so because the class has no implementation to inherit, and is only intended as an interface, which it is now possible to express. This should unblock relanding https://dart-review.googlesource.com/c/sdk/+/287760 (Also working on disabling the class-modifiers experiment for Flutter dart: libraries, which was enabled along with the Dart SDK libraries, until the experiment can be intentionally turned back on.) commit 24afaf9 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon Mar 13 09:44:31 2023 +0000 Bump github/codeql-action from 2.2.5 to 2.2.6 (flutter#40246) Bump github/codeql-action from 2.2.5 to 2.2.6 commit ae979a8 Author: Chinmay Garde <chinmaygarde@google.com> Date: Sun Mar 12 23:49:22 2023 -0700 [Impeller] More sundry fixes to the Vulkan backend. (flutter#40244) [Impeller] More sundry fixes to the Vulkan backend. commit 57f7120 Author: Zachary Anderson <zanderso@users.noreply.github.com> Date: Sat Mar 11 19:38:43 2023 -0800 Add GN arguments that disable building host artifacts (flutter#40242) commit 04e8d54 Author: Chris Bracken <chris@bracken.jp> Date: Sat Mar 11 12:49:59 2023 -0800 [macOS] Forward mouseDown/Up to view controller (flutter#40241) This works around an AppKit bug in which mouseDown/mouseUp events are not correctly forwarded up the responder chain for views nested inside an NSPopover if (and only if) the macOS "Reduce Transparency" accessibility setting is enabled in the System Settings. When the above conditions are satisfied, the nested NSView receives the mouseDown:/mouseUp: call but if it delegates to the default implementation (implemented in NSResponder) mouseDown/mouseUp calls are triggered on containing views (in our case FlutterViewWrapper) but not triggered on the view controller and other responders in the responder chain until we an _NSPopoverWindow class is hit. A minimal AppKit-only (non-Flutter) repro shows this behaviour repros with even a minimal NSViewController implementation and an unmodified NSView. See: https://github.com/cbracken/PopoverRepro A radar has been filed with Apple and a copy posted to OpenRadar. See: http://www.openradar.me/FB12050037 In order to work around this bug, we override mouseDown/mouseUp in the topmost containing view of FlutterView (in our case, FlutterViewWrapper) to have the behaviour documented as the default behaviour in NSResponder's mouseDown/mouseUp documentation. In otherwords, to simply forward the call to self.nextResponder. See: https://developer.apple.com/documentation/appkit/nsresponder/1524634-mousedown Because replicating the exact configuration of a FlutterView contained in an NSPopover and System Settings that have been modified to enable the "Reduce Transparency" setting is difficult and likely error-prone in infra, we instead simulate the bug by testing that even if NSResponder's mouseDown/mouseUp method are swizzled to no-op, these calls are correctly forwarded to the next responder in the chain. If, in the future Apple does fix this issue, this workaround can be removed once Flutter's minimum supported macOS SDK is at least the version that contains the fix. Issue: flutter/flutter#115015 commit c7894a6 Author: Dan Field <dnfield@google.com> Date: Fri Mar 10 16:39:04 2023 -0800 Make the context current before accessing GL in MakeSkiaGpuImage (flutter#40208) Make the context current before accessing GL in MakeSkiaGpuImage commit 7f25023 Author: LongCatIsLooong <31859944+LongCatIsLooong@users.noreply.github.com> Date: Fri Mar 10 15:42:18 2023 -0800 Revert "Make FlutterTest the default test font (flutter#40188)" (flutter#40237) This reverts commit 9270e3d. commit 12f2fdf Author: Yegor <yjbanov@google.com> Date: Fri Mar 10 14:46:06 2023 -0800 Revert "[web] Access engine version to get correct gstatic URL (flutter#40194)" (flutter#40235) This reverts commit 161f759. commit 3018843 Author: skia-flutter-autoroll <skia-flutter-autoroll@skia.org> Date: Fri Mar 10 17:44:48 2023 -0500 Manual roll Dart SDK from 7240b35cc401 to c766fffb626e (9 revisions) (flutter#40233) Manual roll requested by asiva@google.com https://dart.googlesource.com/sdk.git/+log/7240b35cc401..c766fffb626e 2023-03-10 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.0.0-322.0.dev 2023-03-10 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.0.0-321.0.dev 2023-03-10 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.0.0-320.0.dev 2023-03-10 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.0.0-319.0.dev 2023-03-09 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.0.0-318.0.dev 2023-03-09 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.0.0-317.0.dev 2023-03-09 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.0.0-316.0.dev 2023-03-09 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.0.0-315.0.dev 2023-03-09 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.0.0-314.0.dev If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/dart-sdk-flutter-engine Please CC aam@google.com,asiva@google.com,dart-vm-team@google.com,jimgraham@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter Engine: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md commit 220e867 Author: Zachary Anderson <zanderso@users.noreply.github.com> Date: Fri Mar 10 14:38:58 2023 -0800 Roll buildroot to 287917d (flutter#40232) To pick up flutter/buildroot#691 commit c99baf2 Author: Jim Graham <flar@google.com> Date: Fri Mar 10 14:32:52 2023 -0800 Roll Fuchsia Linux SDK to 12.20230309.0.1 (flutter#40231) Roll Fuchsia Linux SDK to 12.20230309.0.1 commit 3b07c4c Author: Jonah Williams <jonahwilliams@google.com> Date: Fri Mar 10 14:24:45 2023 -0800 [Impeller] remove unused shader, format malioc diff (flutter#40230) [Impeller] remove unused shader, format malioc diff
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 10, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 10, 2023
cbracken
added a commit
to cbracken/flutter_engine
that referenced
this pull request
Aug 25, 2024
In flutter#40241, I added a workaround for an AppKit bug wherein mouseDown/Up events were ignored when the *Reduced Transparency* accessibility setting was enabled, and the view was hosted in an `NSPopover`. I filed a Radar and Apple reported the bug fixed in macOS 13.3.1. When we drop support for macOS 12 in Flutter, we should remove the workaround in FlutterViewController, here: https://github.com/flutter/engine/blob/3ce6c4bf17534fddb07ea98df035d33243d9fd7a/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm#L289-L316 So long as nothing else has been added to these methods at that time, the method overrides themselves should be removed, so that we fall back to the default behaviour. See: http://www.openradar.me/FB12050037 See: https://developer.apple.com/documentation/appkit/nsresponder/1535349-mouseup Issue: flutter/flutter#154063 Issue: flutter/flutter#115015
8 tasks
cbracken
added a commit
to cbracken/flutter_engine
that referenced
this pull request
Aug 25, 2024
In flutter#40241, I added a workaround for an AppKit bug wherein mouseDown/Up events were ignored when the *Reduced Transparency* accessibility setting was enabled, and the view was hosted in an `NSPopover`. I filed a Radar and Apple reported the bug fixed in macOS 13.3.1. When we drop support for macOS 12 in Flutter, we should remove the workaround in FlutterViewController, here: https://github.com/flutter/engine/blob/3ce6c4bf17534fddb07ea98df035d33243d9fd7a/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm#L289-L316 So long as nothing else has been added to these methods at that time, the method overrides themselves should be removed, so that we fall back to the default behaviour. See: http://www.openradar.me/FB12050037 See: https://developer.apple.com/documentation/appkit/nsresponder/1535349-mouseup Issue: flutter/flutter#154063 Issue: flutter/flutter#115015
auto-submit bot
pushed a commit
that referenced
this pull request
Aug 25, 2024
In #40241, I added a workaround for an AppKit bug wherein mouseDown/Up events were ignored when the *Reduced Transparency* accessibility setting was enabled, and the view was hosted in an `NSPopover`. I filed a Radar and Apple reported the bug fixed in macOS 13.3.1. When we drop support for macOS 12 in Flutter, we should remove the workaround. So long as nothing else has been added to these methods at that time, the method overrides themselves should be removed, so that we fall back to the default behaviour. See: http://www.openradar.me/FB12050037 See: https://developer.apple.com/documentation/appkit/nsresponder/1535349-mouseup Issue: flutter/flutter#154063 Issue: flutter/flutter#115015 No tests modified since this only adds a comment. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This works around an AppKit bug in which mouseDown/mouseUp events are not correctly forwarded up the responder chain for views nested inside an
NSPopover
if (and only if) the macOS "Reduce Transparency" accessibility setting is enabled in the System Settings.When the above conditions are satisfied, the nested NSView receives the
mouseDown:
/mouseUp:
call but if it delegates to the default implementation (implemented inNSResponder
)mouseDown:
/mouseUp:
calls are triggered on containing views (in our caseFlutterViewWrapper
) but not triggered on the view controller and other responders in the responder chain until we an_NSPopoverWindow
class is hit.A minimal AppKit-only (non-Flutter) repro shows this bug repros with even a minimal
NSViewController
implementation and an unmodifiedNSView
. See: https://github.com/cbracken/PopoverReproA radar has been filed with Apple and a copy posted to OpenRadar. See: http://www.openradar.me/FB12050037
In order to work around this bug, we override mouseDown/mouseUp in the topmost containing view of
FlutterView
(in our case,FlutterViewWrapper
) to have the behaviour documented as the default behaviour inNSResponder
'smouseDown:
/mouseUp:
documentation. In other words, to simply forward the call toself.nextResponder
.See: https://developer.apple.com/documentation/appkit/nsresponder/1524634-mousedown
Because replicating the exact configuration of a
FlutterView
contained in anNSPopover
and System Settings that have been modified to enable the "Reduce Transparency" setting is difficult and likely error-prone in infra, we instead simulate the bug by testing that even ifNSResponder
's mouseDown/mouseUp method are swizzled to no-op, these calls are correctly forwarded to the next responder in the chain.If, in the future Apple does fix this issue, this workaround can be removed once Flutter's minimum supported macOS SDK is at least the version that contains the fix.
Issue: flutter/flutter#115015
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.